Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deterministic for loops for dictionaries. #240

Merged
merged 2 commits into from
Sep 20, 2018

Conversation

djbe
Copy link
Contributor

@djbe djbe commented Sep 20, 2018

⚠️ Note: based on #239, so merge that first!

With Swift 4.2, hashing isn't consistent between runs anymore. A consequence of this is, if we iterate over a dictionary, the output won't always be the same, which will be unexpected for a user.

With this PR we iterate over the sorted keys of a dictionary, ensuring consistent output across runs.

@djbe djbe changed the base branch from master to feature/swift4.2 September 20, 2018 01:06
@djbe djbe merged commit 3f2f1b9 into feature/swift4.2 Sep 20, 2018
@djbe djbe deleted the feature/deterministic-for-loop branch September 20, 2018 22:00
@ilyapuchka
Copy link
Collaborator

ilyapuchka commented Sep 20, 2018

I think it's fine though I don't think this was mentioned anywhere in docs (I mean Stencil) that on dictionaries the order sill be deterministic. But this will preserve current behaviour at least.

@djbe djbe restored the feature/deterministic-for-loop branch September 20, 2018 22:01
@djbe
Copy link
Contributor Author

djbe commented Sep 20, 2018

I think end users would expect consistent behaviour, no matter what the underlying (Swift) behaviour might be?

@djbe
Copy link
Contributor Author

djbe commented Sep 20, 2018

And with the new swift hashing, we're forced to do something ourselves in this case.

@ilyapuchka
Copy link
Collaborator

🤷‍♂️ depends on user understanding of nature of such types =) for some deterministic order in swift < 4.2 could be a surprise too =)

@djbe
Copy link
Contributor Author

djbe commented Sep 20, 2018

@ilyapuchka I inadvertently merged this into the wrong branch 🤦‍♂️, so we should continue our discussion in #242.

@djbe djbe added this to the 0.13.0 milestone Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants